fix: #457 Parameters from `PathItem` can now be overriden in `Operation` #458

mtovts · 2021-07-27T10:47:44Z

This PR aims to fix the code generation with PathItems parameters overriding
in Operation. Please, read more in #457 .

Seems like overriding is job for set and __hash__ overriding. But Property class is
defined with @attr.s(auto_attribs=True, frozen=True). And how to achieve __hash__
overriding with + frozen=True is not obvious. It is the reason for using dict.

Parameter location list-s replaced with dict-s because it allows to
build Endpoint.from_data() with Operation parameters then add PathItem
parameters in EndpointCollection.from_data() by dict.setdefault() to avoid Operation
parameters overriding with PathItem parameters.
Removes from Endpoint._add_parameters
```
return ParseError(data=param, detail="Parameter must be declared in path or query"), schemas
```
Because parameter with location other than listed in ParameterLocation(str, Enum) will
not be validated by pydantic. So why do we need to check that case here twice?
Add more obvious warning in duplicated parameters case.
Always use {param.name}_{param.param_in} python identifier for parameters because it leads
to more obvious usage and simplifies the code.

Please, pay attention to test__add_parameters_happy
in tests\test_parser\test_openapi.py. I'm not sure if I have done it best way there.

Custom templates in end-to-end tests did not pass. I believe I haven't touched them, but it
fails. I made it passing by making imports relative. I ran it on Windows, so maybe it fixes #447 ?

…ries

…ocations` e2e OpenAPI

… tests

…ly if got the parameters with the same name

…on postfix" This reverts commit a4f6458.

This reverts commit 38f341b.

mtovts · 2021-07-31T17:50:00Z

Always use {param.name}_{param.param_in} python identifier for parameters because it leads
to more obvious usage and simplifies the code.

I shouldn’t have changed that behavior. I brought it back and fixed it for this PR.

Replace dict.items() with dict.values() Co-authored-by: Dylan Anthony <[email protected]>

…ests\openapi.json`

mtovts · 2021-08-09T09:39:34Z

@dbanty Thank you for the review! Added your suggestions and a test for the case you described. Also added forgotten used_python_identifiers usage. It is ready now. Should we create a new issue for the behavior I've suggested to resolve identifiers conflict?

dbanty

Thanks for keeping up with this! I have one fix to an error message and a couple suggested refactors for easier readability (subjectively).

openapi_python_client/parser/openapi.py

Co-authored-by: Dylan Anthony <[email protected]>

…ter renaming

mtovts · 2021-08-15T10:36:27Z

@dbanty Dylan, thanks for these improvements! I agree, and also added the missing test 👀

codecov · 2021-08-15T16:19:45Z

Codecov Report

Merging #458 (ca7c30f) into main (52eb8d0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #458   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1589      1592    +3     
=========================================
+ Hits          1589      1592    +3

Impacted Files	Coverage Δ
openapi_python_client/parser/openapi.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52eb8d0...ca7c30f. Read the comment docs.

dbanty

One more tweak, sorry I didn't clarify in the last round.

openapi_python_client/parser/openapi.py

dbanty · 2021-08-15T16:23:17Z

Everything else looks good so I'm gonna try and tweak this myself real quick so you don't have to jump in here again.

dbanty

Thanks again for all your work on this!

mtovts added 27 commits July 22, 2021 11:36

Fix: PathItem parameters overriding in Operation

8994ec3

Update templates\endpoint_* to correct work with parameters

dfc9e5a

Update template to correct work with json_body

1776c89

Update template\endpoint_macros.jinja` to correct work with headers

abe5144

Refactor: Parameters update

7468e4c

Add duplicates parameters check

b1e87e5

code style

2139e38

Refactor: use str instead PythonIdentifier in parameters dictiona…

60cb64f

…ries

Fix: update unit-tests related with Endpoint._add_parameters()

cc55220

Add: common_parameters_overriding endpoint to e2e tests

2247a9a

Add parameters locations: header, cookie in `same-name-multiple-l…

86a9f53

…ocations` e2e OpenAPI

Add generated common_parameters_overriding edpoints to e2e tests

7e34088

Update same_name_multiple_locations_param generated endpoint in e2e…

32e94a1

… tests

Update generated endpoints in common_parameters: add location postfix

a4f6458

Update end_to_end_tests\golden-record\*

38f341b

Update \test_custom_templates\*_init.py.jinja

3292a38

Update test__add_parameters_happy

84a440a

naming

c6bfea2

Delete .PR_note.md

6378616

return: Set parameter identifier {param.name} _ {param.param_in} on…

4080f89

…ly if got the parameters with the same name

Update test related with python identifiers for parameters

237ebb3

Revert "Update generated endpoints in common_parameters: add locati…

bbb4928

…on postfix" This reverts commit a4f6458.

Revert "Update end_to_end_tests\golden-record\*"

d59c555

This reverts commit 38f341b.

Return relative imports in e2e tests

5f171b3

Return: "json_" + endpoint.json_body.python_name

7990d9a

fix property python_name in parameter overriding

2b089a4

remove unused

8d2344c

mtovts added 2 commits August 5, 2021 16:35

Merge branch 'main' into fix/overriding-parameters

ffdacfd

Merge branch 'main' into fix/overriding-parameters

cf9566c

mtovts and others added 8 commits August 8, 2021 00:06

Update openapi_python_client/templates/endpoint_macros.py.jinja

035c491

Replace dict.items() with dict.values() Co-authored-by: Dylan Anthony <[email protected]>

Update openapi_python_client/templates/endpoint_macros.py.jinja

d8c7cb3

Replace dict.items() with dict.values() Co-authored-by: Dylan Anthony <[email protected]>

Fix end_to_end_tests\openapi.json

3e7fe11

Add /same_identifier_conflict/{param}/{param_path} to `end_to_end_t…

b4cfa2e

…ests\openapi.json`

Lint tests\test_parser\test_openapi.py

08b5491

Add test__add_parameters_with_location_postfix_conflict

578ddf2

Add used_python_identifiers usage

0c82680

Lint the code

43774dd

mtovts requested a review from dbanty August 9, 2021 09:42

dbanty requested changes Aug 14, 2021

View reviewed changes

mtovts and others added 8 commits August 14, 2021 21:29

Error message

d4a5cfd

Co-authored-by: Dylan Anthony <[email protected]>

Refactor: skip PathItem parameter if Operation parameter exists

87debe1

Co-authored-by: Dylan Anthony <[email protected]>

Refactor: simplify logic

95b4fa4

Co-authored-by: Dylan Anthony <[email protected]>

Fix spaces

39ab056

Fix tests

28b824b

make line-length < 120

d78d4b0

add test for existing parameter has a conflicting PythonIdentifier af…

a973637

…ter renaming

Refactor test for PythonIdentifier of new parameter already used

a471025

Merge branch 'main' into fix/overriding-parameters

294ceb1

mtovts requested a review from dbanty August 15, 2021 10:44

dbanty requested changes Aug 15, 2021

View reviewed changes

openapi_python_client/parser/openapi.py Outdated Show resolved Hide resolved

refactor: Remove unneeded check

ca7c30f

dbanty approved these changes Aug 15, 2021

View reviewed changes

dbanty enabled auto-merge (squash) August 15, 2021 16:27

dbanty disabled auto-merge August 15, 2021 16:31

dbanty merged commit 53cdd94 into openapi-generators:main Aug 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: #457 Parameters from `PathItem` can now be overriden in `Operation` #458

fix: #457 Parameters from `PathItem` can now be overriden in `Operation` #458

mtovts commented Jul 27, 2021

mtovts commented Jul 31, 2021

mtovts commented Aug 9, 2021

dbanty left a comment

mtovts commented Aug 15, 2021

codecov bot commented Aug 15, 2021 •

edited

Loading

dbanty left a comment

dbanty commented Aug 15, 2021

dbanty left a comment

fix: #457 Parameters from PathItem can now be overriden in Operation #458

fix: #457 Parameters from PathItem can now be overriden in Operation #458

Conversation

mtovts commented Jul 27, 2021

mtovts commented Jul 31, 2021

mtovts commented Aug 9, 2021

dbanty left a comment

Choose a reason for hiding this comment

mtovts commented Aug 15, 2021

codecov bot commented Aug 15, 2021 • edited Loading

Codecov Report

dbanty left a comment

Choose a reason for hiding this comment

dbanty commented Aug 15, 2021

dbanty left a comment

Choose a reason for hiding this comment

fix: #457 Parameters from `PathItem` can now be overriden in `Operation` #458

fix: #457 Parameters from `PathItem` can now be overriden in `Operation` #458

codecov bot commented Aug 15, 2021 •

edited

Loading